src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (2)

49-52: Remove redundant CreateUpdateMonitor call in constructor.

The CreateUpdateMonitor call in the constructor appears redundant as it's called again in getUpdateMonitor() method, and its return value is ignored here.

 public FlatpakUpdater() {
     this.portal = new UpdatePortal();
-    portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
 }

---

173-193: Consider extracting a helper method to reduce code duplication.

The pattern of extracting string values from variants is repeated multiple times.

+private String extractStringFromVariant(Map<String, Variant<?>> map, String key) {
+    Variant<?> variant = map.get(key);
+    return variant != null ? (String) variant.getValue() : "";
+}
+
 private void notifyOnUpdateAvailable(Flatpak.UpdateMonitor.UpdateAvailable signal) {
-    String remoteCommit = "";
-    Variant<?> remoteCommitVariant = signal.update_info.get("remote-commit");
-    if (null != remoteCommitVariant) {
-        remoteCommit = (String) remoteCommitVariant.getValue();
-    }
-    String runningCommit = "";
-    Variant<?> runningCommitVariant = signal.update_info.get("running-commit");
-    if (null != runningCommitVariant) {
-        runningCommit = (String) runningCommitVariant.getValue();
-    }
-    String localCommit = "";
-    Variant<?> localCommitVariant = signal.update_info.get("local-commit");
-    if (null != localCommitVariant) {
-        localCommit = (String) localCommitVariant.getValue();
-    }
+    String remoteCommit = extractStringFromVariant(signal.update_info, "remote-commit");
+    String runningCommit = extractStringFromVariant(signal.update_info, "running-commit");
+    String localCommit = extractStringFromVariant(signal.update_info, "local-commit");
     UpdateAvailable updateAvailable = new UpdateAvailable(runningCommit, localCommit, remoteCommit);
     for (UpdateAvailableListener listener : updateAvailableListeners) {
         listener.onUpdateAvailable(updateAvailable);
     }
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (4)

59-62: Avoid hardcoding the display parameter.

The hardcoded "x11:0" display parameter may not work correctly in Wayland environments or other display configurations.

Consider obtaining the display parameter dynamically:

 @Override
 public void triggerUpdate() throws UpdateFailedException {
 	var monitor = getUpdateMonitor();
-	portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY);
+	String display = System.getenv("DISPLAY");
+	if (display == null || display.isEmpty()) {
+		display = System.getenv("WAYLAND_DISPLAY");
+		if (display != null && !display.isEmpty()) {
+			display = "wayland:" + display;
+		} else {
+			display = "x11:0"; // fallback
+		}
+	}
+	portal.updateApp(display, monitor, UpdatePortal.OPTIONS_DUMMY);
 }

---

82-92: Improve exception handling in close method.

The current implementation catches a generic Exception and the logging could be more informative. Additionally, exceptions should not be swallowed in close() method as per AutoCloseable contract.

 @Override
 public void close() throws Exception {
 	try {
 		if (null != updateMonitor) {
 			portal.cancelUpdateMonitor(updateMonitor);
 		}
 		portal.close();
-	} catch (Exception e) {
-		LOG.error(e.toString(), e.getCause());
+	} catch (DBusException e) {
+		LOG.error("Failed to close DBus connection properly", e);
+		throw e;
+	} catch (Exception e) {
+		LOG.error("Unexpected error during resource cleanup", e);
+		throw e;
 	}
 }

---

124-135: Add type safety checks to prevent ClassCastException.

The direct cast without type checking could throw ClassCastException. Add proper type validation before casting.

 private void notifyOnUpdateProceeds(Flatpak.UpdateMonitor.Progress signal) {
-	long status = ((UInt32) signal.info.get("status").getValue()).longValue();
+	long status = 0;
+	Variant<?> statusVariant = signal.info.get("status");
+	if (statusVariant != null && statusVariant.getValue() instanceof UInt32) {
+		status = ((UInt32) statusVariant.getValue()).longValue();
+	} else {
+		LOG.warn("Missing or invalid status in update progress signal");
+	}
+	
 	long progress = 0;
 	Variant<?> progressVariant = signal.info.get("progress");
-	if (null != progressVariant) {
+	if (progressVariant != null && progressVariant.getValue() instanceof UInt32) {
 		progress = ((UInt32) progressVariant.getValue()).longValue();
 	}
 	Progress p = new Progress(status, progress);
 	for (ProgressListener listener : progressListeners) {
 		listener.onProgress(p);
 	}
 }

---

94-112: Handle monitor creation failure more robustly.

The method can return null if monitor creation fails, which could cause NullPointerException in triggerUpdate(). Additionally, only the Progress signal handler is registered, missing the UpdateAvailable handler mentioned in past reviews.

-private synchronized Flatpak.UpdateMonitor getUpdateMonitor() {
+private synchronized Flatpak.UpdateMonitor getUpdateMonitor() throws UpdateFailedException {
 	if (updateMonitor == null) {
 		var updateMonitorPath = portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
 		if (updateMonitorPath != null) {
 			LOG.debug("UpdateMonitor successful created at {}", updateMonitorPath);
 			updateMonitor = portal.getUpdateMonitor(updateMonitorPath.toString());
 			try {
+				portal.getDBusConnection().addSigHandler(Flatpak.UpdateMonitor.UpdateAvailable.class, signal -> {
+					LOG.info("Update available signal received");
+					// Handle update available notification if needed
+				});
 				portal.getDBusConnection().addSigHandler(Flatpak.UpdateMonitor.Progress.class, signal -> {
 					notifyOnUpdateProceeds(signal);
 				});
 			} catch (DBusException e) {
-				LOG.error(e.toString(), e.getCause());
+				updateMonitor = null; // Reset on error
+				throw new UpdateFailedException("Failed to register DBus signal handlers", e);
 			}
 		} else {
-			LOG.error("Failed to create UpdateMonitor on DBus");
+			throw new UpdateFailedException("Failed to create UpdateMonitor on DBus");
 		}
 	}
 	return updateMonitor;
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: infeo
PR: cryptomator/integrations-linux#80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.

src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (2)

41-44: Remove redundant monitor creation in constructor

portal.CreateUpdateMonitor(...) is invoked eagerly but its result is ignored and a fresh monitor is created later in prepareUpdate(), wasting a DBus round-trip. Drop this call and lazily create the monitor only when needed.

---

99-106: Handle potential DBusException from updateApp and make display selection dynamic

1. portal.updateApp(...) can throw DBusException; wrap the call in try/catch and fail the latch accordingly.
2. The display parameter is hard-coded to "x11:0", which breaks on Wayland or non-standard X11 displays. Obtain it from DISPLAY / WAYLAND_DISPLAY env vars with a fallback.

src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (1)

109-116: Null-safety for signal map access

signal.info.get("progress") is accessed without null-check before the cast at Line 114. Although unlikely, a missing key would raise NullPointerException. Guard with a null check:

Variant<?> progressVariant = signal.info.get("progress");
if (progressVariant != null && progressVariant.getValue() instanceof UInt32 u) {
    progress = u.doubleValue() / 100.0;
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: infeo
PR: cryptomator/integrations-linux#80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.

src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (4)

40-43: Remove unused UpdateMonitor creation in constructor.

The constructor creates an UpdateMonitor at line 42 but doesn't capture or use the result. This is wasteful since firstStep() creates its own monitor when needed. Remove this call and rely on lazy initialization.

Apply this diff:

 public FlatpakUpdater() {
 	this.portal = new UpdatePortal();
-	portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
 }

---

101-110: Avoid hardcoding the display parameter.

Line 109 hardcodes "x11:0" which will fail in Wayland or non-standard X11 display configurations. Obtain the display parameter dynamically from environment variables.

Consider this approach:

+	private static String getDisplayParameter() {
+		String display = System.getenv("DISPLAY");
+		if (display != null && !display.isEmpty()) {
+			return display;
+		}
+		String waylandDisplay = System.getenv("WAYLAND_DISPLAY");
+		if (waylandDisplay != null && !waylandDisplay.isEmpty()) {
+			return "wayland:" + waylandDisplay;
+		}
+		return "x11:0"; // fallback
+	}
+
 	@Override
 	public void start() {
 		try {
 			this.signalHandler = portal.getDBusConnection().addSigHandler(Flatpak.UpdateMonitor.Progress.class, this::handleProgressSignal);
 		} catch (DBusException e) {
 			LOG.error("DBus error", e);
 			latch.countDown();
 		}
-		portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY);
+		portal.updateApp(getDisplayParameter(), monitor, UpdatePortal.OPTIONS_DUMMY);
 	}

---

156-162: Portal lifecycle issue: shared resource closed prematurely.

Line 160 closes the shared portal instance from the parent FlatpakUpdater, making it unusable for subsequent operations. The TODO comment indicates developer uncertainty. Either:

• Let the parent FlatpakUpdater manage portal lifecycle (don't close here), or
• Give each FlatpakUpdateStep its own portal instance

Recommended approach - remove portal closure from cancel:

 	@Override
 	public void cancel() {
 		portal.cancelUpdateMonitor(monitor);
 		stopReceivingSignals();
-		portal.close(); // TODO: is this right? belongs to parent class. update can not be retried afterwards. or should each process have its own portal instance?
 		error = new UpdateFailedException("Update cancelled by user");
 	}

And ensure FlatpakUpdater properly closes the portal when the updater itself is disposed (implement AutoCloseable or add cleanup method).

---

50-72: Add timeout to update availability check.

The cdl.await() call at line 65 blocks indefinitely. If the update check signal never arrives (e.g., due to DBus issues), this will hang forever. Add a reasonable timeout (e.g., 30 seconds) and handle the timeout case.

Apply this diff:

 	try {
-		cdl.await();
-		return updateAvailable.get();
+		if (!cdl.await(30, TimeUnit.SECONDS)) {
+			LOG.warn("Update check timed out after 30 seconds");
+			checkTask.cancel();
+			return false;
+		}
+		return updateAvailable.get();
 	} catch (InterruptedException e) {
 		checkTask.cancel();
 		Thread.currentThread().interrupt();

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java (1)

135-135: Stop hardcoding the X11 display for portal.updateApp.
Wayland sessions (and even non-:0 X11 seats) choke on "x11:0", so updateApp throws and the preparation step aborts immediately. Resolve the display token from DISPLAY/WAYLAND_DISPLAY with a fallback instead of hardcoding the X11 default.

-            portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY);
+            String displayToken = System.getenv("DISPLAY");
+            if (displayToken == null || displayToken.isBlank()) {
+                String waylandDisplay = System.getenv("WAYLAND_DISPLAY");
+                if (waylandDisplay != null && !waylandDisplay.isBlank()) {
+                    displayToken = "wayland:" + waylandDisplay;
+                }
+            }
+            if (displayToken == null || displayToken.isBlank()) {
+                displayToken = "x11:0";
+            }
+            portal.updateApp(displayToken, monitor, UpdatePortal.OPTIONS_DUMMY);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: infeo
Repo: cryptomator/integrations-linux PR: 80
File: src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java:56-62
Timestamp: 2024-07-16T22:36:32.769Z
Learning: For the `FreedesktopAutoStartService` in the Cryptomator project, exceptions are preferred to contain all necessary debugging information without additional logging before throwing them.